metrics: bound ate.sandbox.class with one rule for every emitter - #1484
Conversation
|
Actually, before making this change I have a question about the scheduling - Julian Gutierrez Oschmann (@juli4n) Worker.sandbox_class is optional: substrate/pkg/proto/ateapipb/ateapi.proto Lines 1688 to 1693 in a223367 and it doesn't have a default value. But this field will be used by scheduler to schedule the worker. CreateWorker accepts "" and nothing fills it in. Should we validate it from the API level? |
9bd647a to
365c075
Compare
Jeff Luo (JeffLuoo)
changed the title
|
So, I also want to mention that I think workers will have multiple sandbox classes in the future. Actors will be on a specific sandbox class but the idea that we reserve compute only for gvisor or microVM is an artificial distinction that probably won't hold up as we incrementally improve the dataplane. would that suggest a different approach here? |
Thank you for the context. I patch the PR to drop the change in earlier revision had an empty-class worker inherit its pool's class. This PR only normalizes empty string sandbox class to unknown like other metrics. For the impact of "workers will have multiple sandbox classes in the future" to the metric cc Krisztian F (@krisztianfekete) we may want to revisit some metrics when this happens. |
Krisztian F (krisztianfekete)
left a comment
Krisztian F (krisztianfekete)
left a comment
There was a problem hiding this comment.
Approach looks right to me, two things:
-
The same empty class is still on
ate.actor.crashes.
Same bug as #1475, on an attribute the registry marks required. Maybe we should have a tinySandboxClassAttributehelper in
ateattr so every emitter shares one rule. Fine as a follow up, but please file it
from this PR so it doesn't close along with #1475. -
sum by (pool) (ate_workerpool_workers{state="idle"})still returns 3 for that
pool, because the unknown series keeps the pool labels. Not a regression, as""
counted the same way, but as per #1475 these should not not count as pool capacity, and that only happoens if the query includesclass.
I don't think it changes this PR. We should still do this regardless of how many classes a worker ends up having.
Today, the instrument's whole contract is that the counts are summable, so So I'd expect it to split, so worker counts stay class-less and summable, and maybe eligibility-by-class becomes its own instrument, where overlaps are fine, and no one wants to sum them. That's ~what Not worth designing now while the dataplane is still evolving, and I'd land the label fix first and revisit when the multi-class model becomes stable. Do you mind opening on issue for this? |
Control.CreateWorker does not validate Worker.sandbox_class, so a client can register a worker carrying an empty one, and any worker written before this keeps it. RegisterWorkerCount tallied that raw value, emitting ate.workerpool.workers with ate.sandbox.class="" -- not a member of the attribute's registry vocabulary -- next to the pool's seeded series at 0. Normalize the worker's class with ateattr.NormalizeSandboxClass, so an empty class reports as unknown. That is the rule the same helper already applies to this attribute on the atelet side, so the codebase keeps one rule rather than two. The worker is deliberately not folded into the series of the pool it names. Scheduling matches sandbox_class exactly, so a worker with no class can host no actor; counting it as pool capacity would inflate the idle state and silence an idle == 0 alert while every resume fails for want of a worker. Under unknown it stays visible and alertable, and the pool's own counts stay truthful. The empty class enters through the worker record, not the WorkerPool CR: the CRD defaults sandboxClass to gvisor and rejects "" by enum. Validating Worker.sandbox_class at the API boundary would stop the empty state existing at all; that is a scheduling fix, and a separate change.
The class reaches the emitters from sources that nothing validates, and each emitter decided for itself what to do with a value outside the attribute's vocabulary. Only atelet normalized. So ate.actor.crashes and the ate.actor.lifecycle timers could carry ate.sandbox.class="", which the registry marks required and permits only gvisor, microvm and unknown. The crash counter has a source of its own, separate from the unvalidated worker record: releaseWorker returns an empty class when the worker is already gone or its assignment is already cleared. There the empty means the class could not be read, which is what unknown is for. Add ateattr.SandboxClassAttribute and stamp the attribute through it, so the rule lives in one place. recordSchedulerAssignment keeps its own behavior: it leaves the attribute off when the class is unknown, which its comment explains, and that is a deliberate choice rather than a missing bound.
66ba5a5 to
7ad23ee
Compare
For point 1, the change is not too long so I just do it in this PR. |
Jeff Luo (JeffLuoo)
changed the title
…mment Review fixes. recordEligibleWorkers set ate.sandbox.class from the raw Constraints value. The class comes from sandboxClassString, which gives an empty string for an unspecified enum, thus the same gap the helper closes elsewhere. The attribute is required on ate.scheduler.eligible_workers. Its test asked for the class "kata" to prove a mismatch. That value cannot reach the scheduler: sandboxClassString gives only gvisor, microvm or empty. The test now asks for microvm against a gvisor fleet, which is the same mismatch with a class that the vocabulary permits. The helper's comment named one omit-site. snapshotOp.attrs is a second, and its own comment says so. Also fold the three worker-count tests into a table, and take the class values from the API constants.
Krisztian F (krisztianfekete)
left a comment
Krisztian F (krisztianfekete)
left a comment
There was a problem hiding this comment.
Latest looks good!
Only thing left is docs/, still untouched in the PR. Three instruments produce
unknown now instead of two, so metrics.yaml also need to be updated, + the two lines on the workers note about unknown not being capacity.
Could you also open the multi-class followup so for Ben's thread?
|
Created #1557
Updated |
Four ateapi instruments can now report unknown: the worker count, the crash counter, the lifecycle timers and the eligible-worker histogram. The member's brief described only the atelet source, thus it now names each source. The worker count needs two more lines of its own. A worker with the class unknown is not capacity, thus a query that adds the classes together reports capacity that no resume can use.
|
Krisztian F (@krisztianfekete) can you help merge? Not suppose to be merged by author : ) |
9ad967a
into
agent-substrate:main
3 participants
Fixes #1475
Control.CreateWorkerdoes not validateWorker.sandbox_class, so a client can register a worker with an empty one (and any record written before this keeps it).RegisterWorkerCounttallied that raw value, soate.workerpool.workersemittedate_sandbox_class=""— not a member of theate.sandbox.classregistry vocabulary — alongside the pool's seeded series at 0.This change normalizes the worker's class with the existing
ateattr.NormalizeSandboxClass, so an empty class reports asunknown.New
ateattr.SandboxClassAttribute, used by every emitter. This closes the same gap onate.actor.crashesand theate.actor.lifecycle.*.